Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AP_DDS: Rally Get and Set #28449

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tizianofiorenzani
Copy link
Contributor

@tizianofiorenzani tizianofiorenzani commented Oct 22, 2024

Issue

#28444

What Changed

  • Service /ap/rally_set appends a Rally Point to the list or clears the list (using the clear flag).
    • point: geolocation
    • altitude_frame mandatory, follows enum of AltFrame.
  • Replies with:
  • success is the action is successful;
  • size is the list's size.
  • Service /ap/rally_get gets the Rally Point at a given index. Returns:
    • success
    • size is the list's size.
    • rally the rally point as ardupilot_msgs/Rally type.
  • New ardupilot_msgs/Rally type is defined, that copies the structure definition in AP_Rally.

Test

Auto Test

colcon test --packages-select ardupilot_dds_tests --event-handlers=console_cohesion+ --pytest-args -k test_rally_point

Manual Test

  • run the SITL
  • Call the service to clear the rally list ros2 service call /ap/rally_set ardupilot_msgs/srv/RallySet "{clear: true}"
  • Open up Mavproxy with map and make sure the rally list is clear.
  • Append a new Rally Point: ros2 service call /ap/rally_set ardupilot_msgs/srv/RallySet "{rally: {point: {latitude: -35.36, longitude: 149.17, altitude: 400}}}"
  • In mavproxy, update the rally list and make sure the rally point appears on the map

image

  • use the service call to get the Rally at index 0: ros2 service call /ap/rally_get ardupilot_msgs/srv/RallyGet "index: 0"

Copy link
Collaborator

@Ryanf55 Ryanf55 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice addition, great to see some more interfaces like this.

Tools/ros2/ardupilot_msgs/msg/Rally.msg Outdated Show resolved Hide resolved
Tools/ros2/ardupilot_msgs/msg/Rally.msg Outdated Show resolved Hide resolved
Tools/ros2/ardupilot_msgs/srv/RallySet.srv Outdated Show resolved Hide resolved
Tools/ros2/ardupilot_msgs/srv/RallySet.srv Outdated Show resolved Hide resolved
libraries/AP_DDS/AP_DDS_Client.cpp Outdated Show resolved Hide resolved
@Ryanf55 Ryanf55 added the ROS label Oct 22, 2024
@timtuxworth
Copy link
Contributor

The altitude needs a frame.

@tizianofiorenzani
Copy link
Contributor Author

tizianofiorenzani commented Oct 22, 2024

@timtuxworth I have set the altitude frame explicitly now.
I don't have a clear answer for what to do with the break_altitude: RallyLocation expects that in Above Home level.

@tizianofiorenzani
Copy link
Contributor Author

As there is no code associated with the Rally flags, the DDS interface has been limited to only the Geopoint and the altitude frame

@tizianofiorenzani tizianofiorenzani force-pushed the wips/rally branch 2 times, most recently from 420c0d1 to 6dca61b Compare November 13, 2024 17:03
@tizianofiorenzani
Copy link
Contributor Author

Rebased and pushed again.

Copy link
Collaborator

@Ryanf55 Ryanf55 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionally, looks good. Just needs code cleanup and removal of potential link-flooding.

Tools/ros2/ardupilot_msgs/CMakeLists.txt Outdated Show resolved Hide resolved
Tools/ros2/ardupilot_msgs/CMakeLists.txt Outdated Show resolved Hide resolved
Tools/ros2/ardupilot_msgs/msg/Rally.msg Show resolved Hide resolved
Tools/ros2/ardupilot_msgs/srv/RallyGet.srv Outdated Show resolved Hide resolved
libraries/AP_DDS/AP_DDS_Client.cpp Outdated Show resolved Hide resolved
libraries/AP_DDS/AP_DDS_Client.cpp Outdated Show resolved Hide resolved
libraries/AP_DDS/AP_DDS_Client.cpp Outdated Show resolved Hide resolved
libraries/AP_DDS/AP_DDS_Client.cpp Outdated Show resolved Hide resolved
libraries/AP_DDS/AP_DDS_Client.cpp Outdated Show resolved Hide resolved
@tizianofiorenzani
Copy link
Contributor Author

@Ryanf55 suggestions applied, rebased and tested.

@tizianofiorenzani tizianofiorenzani force-pushed the wips/rally branch 4 times, most recently from 08d2a04 to 3d943b9 Compare November 22, 2024 18:31
@tizianofiorenzani
Copy link
Contributor Author

Rebased, ready for another round.

@tizianofiorenzani
Copy link
Contributor Author

@Ryanf55 @timtuxworth I applied your suggestions. It should be good to go now.

Copy link
Contributor

@timtuxworth timtuxworth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of minor documentation/example fixes. I'm confused why altitude is documented as having 0 digits (which I would question), whereas several examples who altitudes with at least 1 decimal place. For small vehicles 1 meter is a long way.

libraries/AP_DDS/README.md Outdated Show resolved Hide resolved
libraries/AP_DDS/README.md Show resolved Hide resolved
@timtuxworth
Copy link
Contributor

LGTM - thanks for your work on this!

@tizianofiorenzani
Copy link
Contributor Author

@timtuxworth @Ryanf55 could you approve this PR?

Copy link
Contributor

@timtuxworth timtuxworth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

4 participants